Repository navigation
Fix integer overflow in parseString bounds check - #262
arpitjain099 wants to merge 1 commit into
Conversation
parseString read a uint32 length prefix and then checked len(in) against 4+length. That addition is done in uint32, so a length near the maximum wraps: with length 0xFFFFFFFF, 4+length becomes 3, the guard passes for any buffer of 4+ bytes, and the code slices in[4:4+length] = in[4:3], which panics with a slice-bounds error. This is reachable through a malformed pty-req (parsePtyRequest calls parseString on the term field). There is no recover() in the package and the session handler runs in its own goroutine, so the panic takes down the whole process. On servers with no PublicKeyHandler or PasswordHandler set, gliderlabs enables NoClientAuth, so this is unauthenticated there. Advance past the length prefix before the bounds check and compare len(in) against length directly, matching the shape used in golang.org/x/crypto/ssh. Valid parsing is unchanged. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
|
Thanks a lot for finding, documenting, and fixing this issue, @arpitjain099 — especially for the detailed analysis and regression tests. We've ported your fix to our maintained fork of gliderlabs/ssh and merged it here: engity-com/ssh-server-go#4 The original PR and your work are explicitly referenced there so that the origin and credit remain clear. We're maintaining engity-com/ssh-server-go as a continuation of gliderlabs/ssh, since upstream development has unfortunately become rather quiet. Contributions like this are extremely valuable to us, so thank you for putting the work into investigating and fixing this. |
|
Thanks for telling me, and for the attribution. Glad it is useful somewhere that is moving. I will leave this one open in case upstream picks it up, since the fix applies here unchanged. If you would rather I closed it to keep the queue honest about where the work actually landed, say so and I will. |
2 participants
I do supply-chain security research and found this while auditing SSH server parsers.
parseString reads a uint32 length prefix and then guards with
uint32(len(in)) < 4+length. The4+lengthis evaluated in uint32, so a length close to the max wraps around. Withlength = 0xFFFFFFFF,4+lengthbecomes 3, the check passes for any buffer of four or more bytes, and execution reachesin[4 : 4+length]which isin[4:3]and panics with a slice-bounds error.You can hit this with a malformed pty-req: parsePtyRequest feeds the term field straight into parseString. There is no recover() in the package and DefaultSessionHandler runs in its own goroutine, so the panic brings the whole process down rather than just failing the one connection. When a server sets neither a PublicKeyHandler nor a PasswordHandler, NoClientAuth is enabled, so on those setups an unauthenticated client can trigger it.
The fix advances past the length prefix first and then compares
len(in)againstlengthdirectly, which is the same shape golang.org/x/crypto/ssh uses and avoids the addition entirely. Valid inputs parse exactly as before.Added util_test.go: a normal round-trip case, the 0xFFFFFFFF length case, and a malformed pty-req. The oversized cases panic on master and pass with this change;
go test ./...is green after the fix.